feat: restructured metadata provider#72
Conversation
There was a problem hiding this comment.
Pull request overview
This PR restructures external book metadata lookup into a provider-based architecture with a shared port/type layer and an aggregator to combine and rank results across multiple metadata sources.
Changes:
- Introduces
MetadataProviderPort+MetadataProviderIdtypes and shared provider utilities. - Adds Google Books and Open Library metadata provider implementations plus a
MetadataAggregatorService. - Refactors
ExternalBookMetadataServiceto use the aggregator and wires the shared instance into server composition.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sake/src/lib/types/Metadata/Provider.ts | Adds canonical metadata provider ID list + union type. |
| sake/src/lib/server/application/ports/MetadataProviderPort.ts | Defines the provider port, query, and candidate data model. |
| sake/src/lib/server/infrastructure/metadata-providers/metadataProviderUtils.ts | Adds shared parsing/normalization helpers for providers. |
| sake/src/lib/server/infrastructure/metadata-providers/googleBooksMetadataProvider.ts | Implements Google Books provider returning MetadataCandidates. |
| sake/src/lib/server/infrastructure/metadata-providers/openLibraryMetadataProvider.ts | Implements Open Library provider returning MetadataCandidates. |
| sake/src/lib/server/application/services/MetadataAggregatorService.ts | Aggregates providers with per-provider timeout + ranking. |
| sake/src/lib/server/application/services/ExternalBookMetadataService.ts | Switches to aggregator-based lookup and maps candidates to legacy external metadata shape. |
| sake/src/lib/server/infrastructure/metadata-providers/metadataProviderFactory.ts | Adds factory helpers for constructing providers by ID. |
| sake/src/lib/server/config/activatedMetadataProviders.ts | Adds parsing for activated metadata providers from env config. |
| sake/src/lib/server/application/composition.ts | Instantiates and injects shared aggregator/external metadata service into use cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return typeof value === 'string' && value.trim().length > 0 ? value.trim() : null; | ||
| } | ||
|
|
||
| export function asNumber(value: unknown): number | null { |
There was a problem hiding this comment.
asNumber filters out 0 (requires value > 0). This causes valid values like ratingsCount: 0 / ratings_count: 0 to be coerced to null, even though downstream code treats >= 0 as valid (e.g., storing external_rating_count). Consider either allowing 0 here (use >= 0) or introducing separate helpers (e.g., asPositiveNumber for page counts and asNonNegativeNumber for rating counts).
| export function asNumber(value: unknown): number | null { | |
| export function asNumber(value: unknown): number | null { | |
| return typeof value === 'number' && Number.isFinite(value) && value >= 0 ? value : null; | |
| } | |
| export function asPositiveNumber(value: unknown): number | null { |
| return Promise.race([ | ||
| promise, | ||
| new Promise<T>((_, reject) => | ||
| setTimeout(() => reject(new Error(`${label} timed out after ${ms}ms`)), ms) | ||
| ) | ||
| ]); |
There was a problem hiding this comment.
withTimeout uses setTimeout inside Promise.race but never clears the timer when the wrapped promise settles. This can leave stray timers around (and in high traffic, unnecessary work) even when providers respond quickly. Track the timeout handle and clearTimeout it in a finally/cleanup path; if possible, also consider propagating an AbortSignal to providers so the underlying fetch can be cancelled on timeout.
| return Promise.race([ | |
| promise, | |
| new Promise<T>((_, reject) => | |
| setTimeout(() => reject(new Error(`${label} timed out after ${ms}ms`)), ms) | |
| ) | |
| ]); | |
| let timeoutId: ReturnType<typeof setTimeout>; | |
| const timeoutPromise = new Promise<T>((_, reject) => { | |
| timeoutId = setTimeout(() => reject(new Error(`${label} timed out after ${ms}ms`)), ms); | |
| }); | |
| return Promise.race([promise, timeoutPromise]).finally(() => { | |
| clearTimeout(timeoutId); | |
| }); |
| @@ -46,393 +38,64 @@ function pickFirst<T>(...values: Array<T | null | undefined>): T | null { | |||
| return null; | |||
| } | |||
There was a problem hiding this comment.
pickFirst duplicates the behavior of pickFirstValue that was added in metadataProviderUtils.ts. Reusing the shared helper would reduce duplication and keep null/undefined selection logic consistent across metadata code paths.
No description provided.